test: record why testCmdWClosesWindowWhenClosingLastSurfaceInLastWorkspace is skipped - #221
Merged
Conversation
…orkspace The skip carried no reason. Line 19 set STATEFUL_TEST_SKIP with no comment, and the commit that introduced it says nothing about it -- while the quarantine list directly below carries several paragraphs explaining itself. So one test has been silently excluded from CI with no rationale anyone could check, which leaves it in the worst state available: neither fixed nor deleted, and no way to tell which it should be. The likeliest cause was timing. It asserted the window was gone after a single fixed 0.05s run-loop spin -- the same failure mode that made this whole class flaky enough to need a retry. #218 converted it to wait for `window(withId:) == nil` with a 2s budget, so that cause should be gone. Un-skipped to find out. If it fails for some other reason, the skip goes back WITH the reason written down. Refs #186
Un-skipping it (previous commit) answered the question the skip could not: it
does not flake, it fails.
AppDelegateShortcutRoutingTests.swift:1463: failed - Timed out waiting for
Cmd+W on the last surface to close the window
AppDelegateShortcutRoutingTests.swift:1465: XCTAssertNil failed: "<NSWindow: 0x...>"
It waits the full 2s that #218 gave it and the window is still there, so more
time does not help. The shortcut itself dispatches -- the XCTAssertTrue on
debugHandleCustomShortcut just above passes. And headless window closing works
in general: two sibling Cmd+Ctrl+W tests close real windows on the same runner
and are not skipped.
That narrows it to the cascade this test exercises -- close last surface, close
last workspace, close window -- and leaves open whether it is a headless-only
gap or a real bug in a path users rely on.
Restores the skip so CI stays green, with all of that written down. The wiring
is byte-identical to before; the diff is comment-only.
Refs #186
arzafran
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs #186. The experiment ran and gave an answer. Final diff is comment-only.
What this started as
scripts/ci-run-unit-tests.sh:19excluded a test from CI with no comment, and the commit that added it (cf542e67da) says nothing about it — whileQUARANTINED_ON_COMPATthree lines below justifies itself in several paragraphs. One test silently excluded, no rationale on record, no way to tell whether it wanted fixing or deleting.#218 had just replaced its single fixed 0.05s spin with a 2s condition wait, so timing was the obvious suspect. This PR un-skipped it to find out.
What CI answered
It does not flake. It fails. Three things narrow it down:
XCTAssertTrue(debugHandleCustomShortcut(...))immediately above it passes. Routing works; the resulting close doesn't complete.testCmdCtrlWPromptsBeforeClosingWindowand...ClosesWindowAfterConfirmationboth close real windows on the same runner and aren't skipped.So it's specific to the cascade this test exercises: close last surface → close last workspace → close window. Whether that's a headless-only gap or a real bug in a path users rely on is undetermined — nobody has reproduced it outside CI.
What this PR now does
Restores the skip, with all of that written down. The wiring is byte-identical to
main; the diff is comment-only. CI stays green.The important change is that this stops being an unexplained exclusion. The comment says explicitly not to read it as flakiness — it's a known, reproducible failure of user-facing behaviour, parked rather than diagnosed.
Follow-up worth filing
Investigate why the last-surface→last-workspace→window close cascade doesn't complete in the test host. Either it's a headless limitation worth documenting in the test itself, or it's a real bug that only CI has ever noticed.
Test plan
bash -ncleanstatefulandserialmodesgit diff mainon the script is comment-only